Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

WIP: Add cell to CellCache #1060

Draft
wants to merge 1 commit into
base: master
Choose a base branch
from
Draft

WIP: Add cell to CellCache #1060

wants to merge 1 commit into from

Conversation

KnutAM
Copy link
Member

@KnutAM KnutAM commented Sep 19, 2024

Just to ensure that this can be done in a non-breaking way after #798 is merged.

Did not see any performance degradation on the porous media example where the cell type is Union{Quadrilateral, Triangle}, so the approach in this PR does look ok AFAIU.

No need to merge now - just wanted to prevent any issues after 1.0 release..

@termi-official
Copy link
Member

Can you ELI5 why the celltype should be in the CellCache (and why it should be a union)?

Having a union might be an issue for the GPU stuff. We are currently working towards making CellCache type stable for the GPU (xref #1049 (comment)).

Copy link

codecov bot commented Sep 19, 2024

Codecov Report

Attention: Patch coverage is 93.75000% with 1 line in your changes missing coverage. Please review.

Project coverage is 93.62%. Comparing base (31096cb) to head (c16eb1f).
Report is 3 commits behind head on master.

Files with missing lines Patch % Lines
src/iterators.jl 93.75% 1 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##           master    #1060      +/-   ##
==========================================
- Coverage   93.65%   93.62%   -0.03%     
==========================================
  Files          39       39              
  Lines        6003     6023      +20     
==========================================
+ Hits         5622     5639      +17     
- Misses        381      384       +3     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@KnutAM
Copy link
Member Author

KnutAM commented Sep 19, 2024

Needed to support reinit! for vector interpolations using the CellIterator.

Union: the grid has a Union, the cellCache not, but the constructor should be type unstable. Iteration is not.

@termi-official
Copy link
Member

Thanks!

Needed to support reinit! for vector interpolations using the CellIterator.

reinit! acts on some FEValues and you can obtain the information from the geometric interpolation, right?

@KnutAM
Copy link
Member Author

KnutAM commented Sep 19, 2024

reinit! acts on some FEValues and you can obtain the information from the geometric interpolation, right?

The actual cell is required to determine the global direction of faces or edges based on its node ids during reinit of FEValues with non-identity mappings.

@fredrikekre
Copy link
Member

Couldn't the cell be looked up in reinit! instead? We have the cellid

@KnutAM
Copy link
Member Author

KnutAM commented Sep 20, 2024

Couldn't the cell be looked up in reinit! instead? We have the cellid

Yes, would be possible. But that felt like a split design, to partly reinit the cell cache, and then do more lookup based on the grid in the cache during reinit of the fe values.

And also not sure if there are performance pitfalls regarding type stability here, especially for GPU if a Union isn't allowed during reinit!, but haven't checked/thought carefully about this yet.

@fredrikekre fredrikekre added this to the v1.0.0 milestone Sep 26, 2024
@fredrikekre
Copy link
Member

We should not consider type params as part of the API so this is not breaking.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants